Skip to content

DO NOT MERGE: fix(folders): enforce FolderSearchParams required fields in the canonical constructor (#34154) - #36967

Open
fabrizzio-dotCMS wants to merge 3 commits into
mainfrom
issue-34154-java25-record-design-notes
Open

DO NOT MERGE: fix(folders): enforce FolderSearchParams required fields in the canonical constructor (#34154)#36967
fabrizzio-dotCMS wants to merge 3 commits into
mainfrom
issue-34154-java25-record-design-notes

Conversation

@fabrizzio-dotCMS

@fabrizzio-dotCMS fabrizzio-dotCMS commented Aug 7, 2026

Copy link
Copy Markdown
Member

Summary

FolderSearchParams kept its siteId and user null checks in Builder.build(). This moves them into
the canonical constructor, and documents the shape trade-off that made the misplacement easy to miss.

Why the builder could never enforce it

A record's canonical constructor cannot be declared less accessible than the record itself — the
language forbids it. So for a public record there is always a public positional entry point, and
builder() can only ever be a convenience, never a gate:

FolderSearchParams.builder().user(user).build();   // rejected: no siteId
new FolderSearchParams(name, path, false, null, user, ...);   // before: accepted

This is a real difference from the Immutables-based models used elsewhere in this codebase, where the
generated constructor is private (ImportResultprivate ImportResult(...)) and the builder
therefore genuinely is the only door. With a record, the invariant has exactly one enforceable home.

What changed

Both checks moved to the canonical constructor. build() now delegates instead of carrying its own copy.

Behaviour is unchanged for existing callers: same exception type (NullPointerException, via
Objects.requireNonNull), same messages ("siteId is required" / "user is required"), and build()
still rejects the same inputs. Nothing in the repository constructs the record directly other than
build() itself, so no call site changes.

The javadoc note

The same commit range documents what validation cannot fix here, because it is the more interesting
half. The record carries eleven flat components, and the count has already grown once
(includePermissions arrived with #36889). Three pairs of adjacent components share a type —
name/path, limit/offset, orderBy/orderDirection — and three more are boolean
(recursive, respectFrontendRoles, includePermissions).

A transposed argument list therefore compiles and silently searches for the wrong thing, or quietly
flips a permission behaviour. Constructor validation does nothing for that: each transposed value is
individually valid, so there is nothing for a check to reject. The grouping that would fix it by typing
instead of by discipline is recorded in the javadoc:

public record FolderSearchParams(
        FolderCriteria criteria,   // name, path, recursive
        Requester requester,       // user, respectFrontendRoles, includePermissions
        PageRequest page) {        // limit, offset, sortColumn, sortDirection
}

Deliberately not applied: this type is in the FolderAPI.searchFolders signature, so reshaping it is
a public API change and belongs in its own PR. Written down so that keeping the flat shape stays a
decision rather than an oversight.

Testing

44 green — 15 unit, 29 integration.

Test Covers Result
FolderSearchParamsTest the invariants, new 5/5
FolderSearchPaginatorTest the only caller, unchanged 10/10
FolderAPIImplFilterTest folder search through the API 16/16
FolderFactoryImplFilterTest the SQL the params drive 7/7
FolderCollectionDataFetcherTest the GraphQL consumer 6/6

The test that carries the weight is test_directConstruction_enforcesRequiredFields — it exercises the
path a check in build() cannot cover, and would have passed silently before this change. The others
pin what must not have shifted: the same exception and message coming out of the builder, the builder's
defaults, and that the optional components may still be null (name == null is how "no name filter" is
expressed).

doclint is disabled project-wide (parent/pom.xml<doclint>none</doclint>), so the javadoc carries
no build risk.

Breaking Changes

None for any caller in this repository. Strictly speaking, external code that constructed the record
directly with a null siteId or user would now fail at construction rather than later and elsewhere —
which is the intent, and the type is on a public API, so it is worth stating rather than glossing over.

Context

Groundwork for the Devoxx Belgium 2025 Lunch and Learn (#34154). Its dotCMS half argues about when
records fit this codebase and when they do not, and this type carries both halves of that argument at
once: the invariant belongs on the canonical constructor because it cannot be hidden, and eleven flat
components is the point where a record stops being the right carrier.

This PR fixes: #34154

🤖 Generated with Claude Code

Documentation only. No behaviour, signature or field changes.

FolderSearchParams carries ten flat components. Because the record is public, its
canonical constructor cannot be declared less accessible than the record itself,
so there is always a callable ten-argument positional entry point and builder()
is a convenience rather than a gate.

Several components sit adjacent to another of the same type: name/path,
recursive/respectFrontendRoles, limit/offset. A transposed argument list
therefore compiles and silently searches for the wrong thing. Validating inside
the canonical constructor does not help with that, because each transposed value
is individually valid and there is nothing for a check to reject.

The javadoc records the grouping that would remove the hazard by typing rather
than by discipline (FolderCriteria / Requester / PageRequest), together with the
reason it is not applied: the type appears in the FolderAPI.searchFolders
signature, so reshaping it is a public API change. Written down so that keeping
the flat shape stays a deliberate choice rather than an oversight.

Also noted, and independent of the shape: the siteId and user null checks live in
Builder.build(), which a direct call to the canonical constructor bypasses.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @fabrizzio-dotCMS's task in 41s —— View job


Code Review

  • Read the diff (FolderSearchParams.java, FolderSearchParamsTest.java)
  • Verify construction paths and behaviour preservation
  • Check for bugs, conventions, and test gaps

No issues found.

The change is correct and well-scoped:

  • Validation move is sound. Both Objects.requireNonNull checks moved from Builder.build() to the canonical constructor (FolderSearchParams.java:66-69), and build() now delegates (:105-108). Same exception type (NullPointerException) and messages ("siteId is required" / "user is required"), so behaviour is unchanged for every existing caller. The claim that the canonical constructor is the only unbypassable path is accurate for a public record.

  • No missed call sites. The only non-test new FolderSearchParams(...) in the repo is build() itself, so moving the checks upstream can't regress any direct construction path.

  • Tests pin the right things. test_directConstruction_enforcesRequiredFields (FolderSearchParamsTest.java:37) exercises exactly the gap a build()-local check couldn't cover, and test_optionalComponentsMayBeNull guards against the move accidentally tightening the optional components. The assertSame vs assertEquals note on user() (:92-93) is a correct call — a bare User has no primary key for equals().

  • No build risk from the javadoc. doclint is none project-wide, and the aspirational FolderCriteria/Requester/PageRequest types appear only inside a <pre>{@code ...} block, so they are not resolved @link targets.

One tiny cosmetic note (non-blocking, not a finding): the illustrative snippet comments PageRequest as // limit, offset, sortColumn, sortDirection while the real components are orderBy / orderDirection. Harmless in an example block, but you may want the names to match if this design ever gets applied.

The PR title still says DO NOT MERGE — presumably intentional while this is Devoxx groundwork.
· issue-34154-java25-record-design-notes

🤖 Review by Claude Opus 4.8

@fabrizzio-dotCMS fabrizzio-dotCMS changed the title docs(folders): record the shape trade-off on FolderSearchParams (#34154) DO NOT MERGE: docs(folders): record the shape trade-off on FolderSearchParams (#34154) Aug 7, 2026
…ical constructor (#34154)

The siteId and user null checks lived in Builder.build(). A record's canonical
constructor cannot be declared less accessible than the record itself, so for a
public record there is always a public positional entry point and the builder can
only ever be a convenience, never a gate. A check in build() guarded the callers
who happened to use the builder; new FolderSearchParams(...) walked past it.

Moved both checks into the canonical constructor, which every construction path
goes through, the builder's own included.

Behaviour is unchanged for existing callers: same exception type
(NullPointerException, via Objects.requireNonNull), same messages, and build()
still rejects the same inputs because it now delegates instead of carrying its own
copy of the check. Nothing in the repository constructs the record directly other
than build() itself, so no caller changes.

Tests: 15 green.
- FolderSearchParamsTest (5 new unit tests). The one that matters is
  test_directConstruction_enforcesRequiredFields: it exercises the path a check in
  build() cannot cover, and would have passed silently before this change. The rest
  pin what must NOT have shifted - the same exception and message from the builder,
  the builder's defaults, and that the optional components may still be null.
- FolderSearchPaginatorTest (10 existing) unchanged and passing.

The javadoc design note added earlier in this branch is updated to describe where
the invariants now live and why, instead of flagging them as bypassable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@fabrizzio-dotCMS fabrizzio-dotCMS changed the title DO NOT MERGE: docs(folders): record the shape trade-off on FolderSearchParams (#34154) fix(folders): enforce FolderSearchParams required fields in the canonical constructor (#34154) Aug 7, 2026
@fabrizzio-dotCMS fabrizzio-dotCMS changed the title fix(folders): enforce FolderSearchParams required fields in the canonical constructor (#34154) DO NOT MERGE: fix(folders): enforce FolderSearchParams required fields in the canonical constructor (#34154) Aug 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area : Backend PR changes Java/Maven backend code

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[TASK] Lunch and Learn — Devoxx Belgium 2025: Java 21→25 in the dotCMS codebase

1 participant